Conversation
…Config backup_dir was only consumed by the migration-time backup. The nested BackupConfig (Enabled/IntervalMinutes/BackupDir) was never read — periodic backups are not wired up anywhere. Remove the YAML field, the /tmp/ap-avs-backup default, and the dead struct. The migrator now writes to <db_path>_backup automatically, keeping backups co-located with the DB instead of falling back to /tmp.
The Simulation:/Run #N:/Run Node: prefix now renders inside the code block alongside the workflow name, restoring the pre-rename behavior where the whole "Simulation: WorkflowName" chunk was code-wrapped.
Code ReviewOverviewThis PR contains two independent fixes:
Both changes are well-scoped and easy to follow. Telegram Subject Code-Wrap (
|
| # | Severity | Location | Issue |
|---|---|---|---|
| 1 | Minor bug | config/aggregator.example.yaml |
Comment says <db_path>-backup but code produces <db_path>_backup (hyphen vs underscore) |
| 2 | Nit | core/config/config.go |
Empty DbPath produces relative _backup path; old default was absolute /tmp/ap-avs-backup |
| 3 | Info | Deployment | backup_dir YAML removal is a silent breaking change for any operator config that had it set |
The Telegram fix and dead-code removal are clean. Fix the comment inconsistency (issue #1) before merging.
There was a problem hiding this comment.
Pull request overview
This PR updates Telegram notification formatting to code-wrap the full subject prefix + workflow name, and simplifies migration-time database backup configuration by deriving the backup directory from db_path and removing unused backup config fields.
Changes:
- Wrap Telegram subject prefix + workflow name together inside a single
<code>block for consistent rendering. - Remove unused
BackupConfig/YAML fields and derive migration backup path directly fromdb_path. - Update/align tests and config examples with the new formatting and backup behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core/taskengine/summarizer_format_test.go | Updates expected Telegram output strings to match the new <code> wrapping behavior. |
| core/taskengine/summarizer_format_telegram.go | Changes subject formatting to wrap prefix + workflow name within <code>. |
| core/config/config.go | Removes dead backup config structs/fields and derives BackupDir from DbPath. |
| config/aggregator.example.yaml | Removes backup_dir and documents the new derived backup directory behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| environment: development | ||
|
|
||
| ## Aggregator Server Configs | ||
| # Migration backups are written to <db_path>-backup automatically. |
| DbPath: configRaw.DbPath, | ||
| BackupDir: configRaw.BackupDir, | ||
| BackupDir: configRaw.DbPath + "_backup", | ||
| JwtSecret: []byte(configRaw.JwtSecret), |
The comment said <db_path>-backup but the code derives <db_path>_backup. Caught by both Copilot and Claude review on PR #529.
backup_dir was only consumed by the migration-time backup. The nested
BackupConfig (Enabled/IntervalMinutes/BackupDir) was never read — periodic
backups are not wired up anywhere. Remove the YAML field, the
/tmp/ap-avs-backup default, and the dead struct. The migrator now writes
to <db_path>_backup automatically, keeping backups co-located with the DB
instead of falling back to /tmp.
blockThe Simulation:/Run #N:/Run Node: prefix now renders inside the code
block alongside the workflow name, restoring the pre-rename behavior
where the whole "Simulation: WorkflowName" chunk was code-wrapped.